fuzz: model chanmon mempool mining#4657
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
|
@wpaulino FYI, this may intersect with the existing splice fuzzing failures you’re looking at. This PR makes splice transaction mining in |
joostjager
force-pushed
the
chanmon-mempool-mining
branch
from
b9a40e5 to
bbbd224
Compare
Restore cfg(splicing) to the fuzz check-cfg allow list and gate chanmon consistency splice opcodes on that cfg again. Without the cfg, those inputs stop before executing splice-specific operations.
joostjager
force-pushed
the
chanmon-mempool-mining
branch
3 times, most recently
from
4e6f262 to
d02194d
Compare
|
Fuzz failure is pre-existing |
joostjager
force-pushed
the
chanmon-mempool-mining
branch
2 times, most recently
from
0ab5882 to
ef0be5b
Compare
| // Connects this node from its tracked height to target_height, delivering | ||
| // each relevant chain callback to both ChainMonitor and ChannelManager. | ||
| fn connect_chain_range(&mut self, chain_state: &ChainState, target_height: u32) { | ||
| // Mining commands can advance the harness chain by more than one block. | ||
| // Transaction blocks must be connected explicitly so LDK learns about | ||
| // on-chain spends, while empty depth blocks still need best-block | ||
| // updates so CSV/CLTV-sensitive logic can run. This is the normal sync | ||
| // path, so both the raw ChainMonitor and the ChannelManager receive the | ||
| // callbacks and the node's tracked height advances to the target. | ||
| let mut height = self.height; | ||
| while height < target_height { | ||
| let mut next_height = height + 1; | ||
| while next_height <= target_height && chain_state.block_at(next_height).1.is_empty() { | ||
| next_height += 1; | ||
| } | ||
| if next_height > target_height { | ||
| // The rest of the range is empty. One best-block update to the | ||
| // final height is enough because LDK's Confirm API explicitly | ||
| // allows best_block_updated to skip intermediary blocks, and | ||
| // empty blocks have no transactions_confirmed calls whose chain | ||
| // order must be preserved. | ||
| height = target_height; | ||
| let (header, _) = chain_state.block_at(height); | ||
| self.monitor.best_block_updated(header, height); | ||
| self.node.best_block_updated(header, height); | ||
| break; | ||
| } | ||
| if next_height > height + 1 { | ||
| // Advance across the empty prefix before the next transaction | ||
| // block. Confirm::best_block_updated may skip intermediary | ||
| // blocks, so this compressed update still lets height-triggered | ||
| // LDK work run at the correct tip before the transaction | ||
| // confirmations are connected. | ||
| height = next_height - 1; | ||
| let (header, _) = chain_state.block_at(height); | ||
| self.monitor.best_block_updated(header, height); | ||
| self.node.best_block_updated(header, height); | ||
| } | ||
| height = next_height; | ||
| let (header, txn) = chain_state.block_at(height); | ||
| let txdata: Vec<_> = txn.iter().enumerate().map(|(i, tx)| (i + 1, tx)).collect(); | ||
| if !txdata.is_empty() { | ||
| // Transaction blocks need the explicit confirmation callback | ||
| // before the best-block update so watched spends are delivered | ||
| // in chain order before the node advances to that tip. | ||
| self.monitor.transactions_confirmed(header, &txdata, height); | ||
| self.node.transactions_confirmed(header, &txdata, height); | ||
| } | ||
| self.monitor.best_block_updated(header, height); | ||
| self.node.best_block_updated(header, height); | ||
| } | ||
| self.height = target_height; | ||
| } |
There was a problem hiding this comment.
Good improvement: the old sync_with_chain_state only notified self.node (ChannelManager), never self.monitor (ChainMonitor). This means ChannelMonitors weren't receiving transactions_confirmed / best_block_updated callbacks during normal fuzz-loop sync, which would have hidden any bugs in monitor chain-tracking logic. The new connect_chain_range fixing this is an important correctness improvement.
One behavioral change worth noting: the old code called best_block_updated for every individual block height. The new code batches consecutive empty blocks into a single best_block_updated call at the last empty height before the next tx block (or at the target). This is allowed by the Confirm trait ("May be skipped for intermediary blocks"), but it means height-triggered logic inside ChainMonitor/ChannelManager runs at fewer checkpoints than before. For a fuzzer seeking maximum coverage of height-sensitive paths (e.g., HTLC timeout detection), individual block delivery would exercise more code paths.
There was a problem hiding this comment.
You could also argue that skipping exercises different paths. I think in practice, we'll get a bit from both.
|
I've completed a thorough re-review of the entire PR diff. The prior review already captured all the significant issues. The assert guard for No new issues found. All prior review comments remain applicable (the issues they flagged are present in the current code). Here is my final summary: Review SummaryAfter a thorough re-review of the entire diff (all hunks, all files), no new issues were found beyond what was already flagged in the prior review pass. Prior inline comments (still applicable, not re-posted)
Observations (not issues)
|
joostjager
force-pushed
the
chanmon-mempool-mining
branch
from
ef0be5b to
4036d6a
Compare
Route chanmon broadcasts through an explicit harness mempool. Relay, mining, wallet updates, and chain delivery share one path. This lets splice, anchor, and claim txs enter the mempool before mining.
joostjager
force-pushed
the
chanmon-mempool-mining
branch
from
4036d6a to
57960f1
Compare
3 participants
This prepares
chanmon_consistencyfor force-close fuzzing by making its chain model closer to the environment LDK sees in normal operation.Force-close scenarios depend heavily on transaction timing: claims may be broadcast, replaced, confirmed, followed by additional claims, and later become spendable only after more blocks. The previous harness mostly folded transaction confirmation into sync-style actions, which made it harder to express those flows accurately and made future force-close coverage depend on shortcuts in the test harness.
The updated model gives the harness an explicit mempool and block-mining path. Broadcast transactions can be relayed into the modeled mempool, mined into harness blocks, and then replayed to both monitors and managers through chain callbacks. The harness also tracks confirmed UTXOs and wallet change so later splice, anchor, and claim transactions have a realistic view of what can be spent.
This should make upcoming force-close fuzzing changes easier to review: first establish a more faithful chain environment, then add the force-close-specific scenarios and invariants on top of it.